Skip to content

chore: improve onboarding flow#2128

Open
mxkaske wants to merge 11 commits intomainfrom
chore/improve-onboarding-flow
Open

chore: improve onboarding flow#2128
mxkaske wants to merge 11 commits intomainfrom
chore/improve-onboarding-flow

Conversation

@mxkaske
Copy link
Copy Markdown
Member

@mxkaske mxkaske commented May 1, 2026

No description provided.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
openstatus-dashboard Ready Ready Preview, Comment May 4, 2026 6:49am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
openstatus-status-page Skipped Skipped May 4, 2026 6:49am
openstatus-web Skipped Skipped May 4, 2026 6:49am

Request Review

@openstatusHQ openstatusHQ deleted a comment from claude Bot May 1, 2026
@mxkaske mxkaske marked this pull request as ready for review May 1, 2026 18:07
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 42 files

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 3, 2026

Deploying openstatus-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8c1399f
Status: ✅  Deploy successful!
Preview URL: https://00685dfe.openstatus-1sy.pages.dev
Branch Preview URL: https://chore-improve-onboarding-flo.openstatus-1sy.pages.dev

View logs

@vercel vercel Bot temporarily deployed to Preview – openstatus-web May 3, 2026 14:47 Inactive
@vercel vercel Bot temporarily deployed to Preview – openstatus-status-page May 3, 2026 14:47 Inactive
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 43 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/dashboard/src/components/onboarding/checks-table-adapter.ts">

<violation number="1" location="apps/dashboard/src/components/onboarding/checks-table-adapter.ts:77">
P2: Do not replace the measured latency with summed phase durations; this can show incorrect latency values in onboarding results.</violation>
</file>

<file name="apps/dashboard/src/components/onboarding/checks-table.tsx">

<violation number="1" location="apps/dashboard/src/components/onboarding/checks-table.tsx:159">
P2: Use the count of visible columns for `colSpan` instead of total defined columns.</violation>
</file>

<file name="apps/dashboard/src/data/onboarding-checks.ts">

<violation number="1" location="apps/dashboard/src/data/onboarding-checks.ts:38">
P2: Avoid using `Date.now()` for pre-rendered mock rows; it can produce different server/client timestamp output during hydration. Use a fixed seed timestamp so initial HTML stays stable.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/dashboard/src/components/onboarding/checks-table-adapter.ts Outdated
Comment thread apps/dashboard/src/components/onboarding/checks-table.tsx Outdated
Comment thread apps/dashboard/src/data/onboarding-checks.ts Outdated
@openstatusHQ openstatusHQ deleted a comment from claude Bot May 3, 2026
@mxkaske
Copy link
Copy Markdown
Member Author

mxkaske commented May 3, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 3, 2026

Claude finished @mxkaske's task in 3m 26s —— View job


Code Review: chore/improve-onboarding-flow

Tasks

  • Gather context and understand PR scope
  • Review core onboarding steps and layout
  • Review streaming/data layer
  • Review components and forms
  • Post final review

This is a well-structured PR. The 3-step flow is clean, the streaming preview adds a compelling "magic moment," and the issues flagged by the previous AI review (phase-sum latency, colSpan, Date.now seed) have all been addressed. Notes below go from most to least significant.

P2 — Substantive

1. Dashboard theme is permanently mutated when the user changes "Mode" then skips
create-page.tsx:211-215

onValueChange={(v) => {
  field.onChange(v);
  setDashboardTheme(v);  // ← persists even if user hits "Do this later"
}}

If the user flips to Dark in the preview and then skips Step 2, their dashboard stays dark. The original theme should be saved and restored on skip/unmount. A useRef for the pre-entry theme + an onSkip callback that calls setDashboardTheme(originalRef.current) would fix this, or the side effect could be scoped to the preview component itself rather than the global theme. Fix this →


2. No rate limiting on /api/onboarding/checks
route.ts

Each call fans out to all ~28 active regions. There's no debounce, cooldown, or rate-limit on the endpoint — a single authenticated user could fire it in a tight loop. The retry button in checks-table.tsx is client-side only, so nothing prevents programmatic abuse. Even a simple per-user per-minute check (or an AbortSignal tied to the monitor ID to cancel a previous in-flight stream before starting a new one) would reduce the blast radius.


3. Second rejection in yieldMany silently overwrites the first
packages/utils/src/stream.ts:20-24

(error) => {
  rejection = { error };  // ← unconditionally overwrites a prior rejection
  pending--;
  wake?.();
},

If two promises reject in the same microtask batch, only the last error survives. For the checker fan-out, individual failures return error-shaped CheckResult objects rather than throwing, so this is rarely hit in production — but it's a silent contract violation worth guarding:

if (!rejection) rejection = { error };

P3 — Minor

4. Locked preview in Step 2 shows stale draft components, not the submitted state
step-2.tsx:119

components={previewValues.components ?? []}

previewValues is only updated through the form's onValuesChange. When isLocked === true, the preview keeps rendering whatever the user last typed, not the saved components. For a decorative preview it's low-risk, but snapshotting previewValues at submit time and using that in the locked branch would be more correct.


5. getServiceContextFromRequest has an unreachable fallback
edge-context.ts:47-50

const activeWorkspace =
  userAndWorkspace.usersToWorkspaces?.find(({ workspace }) => {
    if (workspaceSlugCookie) return workspace.slug === workspaceSlugCookie;
    return true;                     // ← returns the first element
  })?.workspace ?? userAndWorkspace.usersToWorkspaces?.[0]?.workspace;
  //              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  //              unreachable: find with `return true` already returns [0]

The ?? [0]?.workspace fallback is dead code — find(() => true) already returns the first element. This doesn't cause a bug but could mislead a future reader into thinking the two branches have different semantics.


6. emitOnboardingTelemetry calls .mutate() twice on the same instance
client.tsx:170-192

useMutation tracks a single in-flight state; calling .mutate() a second time before the first resolves cancels the first's callbacks. These are fire-and-forget telemetry writes so a dropped callback rarely matters, but using two separate useMutation instances (or mutateAsync + Promise.allSettled) would be more correct.


7. PLACEHOLDER_BAR_DATA computed once at module load time
status-page-preview.tsx:63-76

new Date() is called when the module first loads. On a long-lived session the "45-day" window drifts without a page refresh. For purely decorative placeholder data this is inconsequential, but a useMemo with a stable reference date (e.g. today at mount time) would keep the window current across sessions.


8. safeHostname declared after its call sites
client.tsx:315

It's used at lines 229 and 255 but declared at the bottom of the file. Function declarations are hoisted so this works, but moving it above Client() (or beside slugifyHostname) follows the "define before use" convention and makes the dependency obvious at a glance.


Positive callouts

  • The checksStartedRef guard (client.tsx:110-150) correctly prevents double-streaming when React strict-mode double-invokes effects.
  • The callbackUrl redirect validation (client.tsx:153-164) is solid — same-origin check + protocol guard + no external-origin navigation.
  • The yieldMany settle-as-they-arrive queue implementation avoids the classic Promise.race splice-drop bug and the comment explains the invariant clearly.
  • Fixed issues from the prior review are all confirmed resolved (latency now uses result.latency, colSpan now uses table.getVisibleLeafColumns().length, now uses a fixed seed date).
  • getServiceContextFromRequest correctly scopes all reads to the authenticated user's workspace.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 14 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/dashboard/src/components/forms/status-page/theme-picker.tsx">

<violation number="1" location="apps/dashboard/src/components/forms/status-page/theme-picker.tsx:40">
P2: Using `resolvedTheme` directly during initial render can cause hydration mismatch for swatch colors. Gate theme-dependent rendering until mount (or render a stable fallback first) before switching to dark/light palette.</violation>
</file>

<file name="apps/dashboard/src/app/onboarding/_steps/step-2.tsx">

<violation number="1" location="apps/dashboard/src/app/onboarding/_steps/step-2.tsx:51">
P2: Locking step 2 depends on transient mutation data, so a refresh can unlock the form even when `pageStatus` is already `completed`.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/dashboard/src/components/forms/status-page/theme-picker.tsx Outdated
Comment thread apps/dashboard/src/app/onboarding/_steps/step-2.tsx
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 7 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/dashboard/src/components/forms/onboarding/create-page.tsx">

<violation number="1" location="apps/dashboard/src/components/forms/onboarding/create-page.tsx:88">
P2: The original dashboard theme is snapshotted too early and can remain `undefined`, so unmount may fail to restore the user’s prior theme.</violation>
</file>

<file name="apps/dashboard/src/lib/edge-context.ts">

<violation number="1" location="apps/dashboard/src/lib/edge-context.ts:48">
P2: When `workspace-slug` is set but invalid, this no longer falls back to the first workspace, causing `activeWorkspace` to become `undefined` and the request context to return `null`.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/dashboard/src/components/forms/onboarding/create-page.tsx Outdated
Comment thread apps/dashboard/src/lib/edge-context.ts
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/services/src/monitor/__tests__/stream-monitor-preview.test.ts">

<violation number="1" location="packages/services/src/monitor/__tests__/stream-monitor-preview.test.ts:52">
P2: Use `delete` here instead of assigning `undefined`; otherwise the env var is not actually restored to the unset state.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@vercel vercel Bot temporarily deployed to Preview – openstatus-status-page May 4, 2026 06:47 Inactive
@vercel vercel Bot temporarily deployed to Preview – openstatus-web May 4, 2026 06:47 Inactive
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/dashboard/src/components/onboarding/status-page-preview.tsx">

<violation number="1" location="apps/dashboard/src/components/onboarding/status-page-preview.tsx:123">
P2: The new `!mounted` early return prevents `previewRef` from being established before `mounted` flips, so `StatusBar` may never receive the intended portal container.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

.map((c, i) => ({ key: `c-${i}`, name: c.name })),
];

if (!mounted) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The new !mounted early return prevents previewRef from being established before mounted flips, so StatusBar may never receive the intended portal container.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/src/components/onboarding/status-page-preview.tsx, line 123:

<comment>The new `!mounted` early return prevents `previewRef` from being established before `mounted` flips, so `StatusBar` may never receive the intended portal container.</comment>

<file context>
@@ -119,6 +120,12 @@ export function OnboardingStatusPagePreview({
       .map((c, i) => ({ key: `c-${i}`, name: c.name })),
   ];
 
+  if (!mounted) {
+    return (
+      <Skeleton className={cn("rounded-md border border-border", className)} />
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant